-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Allow adding the same dataset multiple times to a bouquet. #507
feat: Allow adding the same dataset multiple times to a bouquet. #507
Conversation
…warning on selection. Removed max-height on Multiselect. Forced badge icon from node_modules
✅ Deploy Preview for meteo-france ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ecospheres ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merci !
J'ai un petit doute sur le "Déjà sélectionné". C'est effectivement une info qui peut être intéressante mais :
- le warning peut laisser à penser que c'est un problème, hors ce n'est pas franchement le cas
- je ne sais pas si le message est suffisamment compréhensible, i.e. raccourci de "Déjà sélectionné dans ce bouquet" (un peu long pour un badge)
Alternatives :
- oublier cet avertissement
- utiliser un texte à côté de la carte du jeu de données, ce qui nous laisserait plus de place pour mieux expliquer
Si on reste là-dessus, on peut essayer de reprendre le placement du badge "Archivé" sur une carte jeu de données data.gouv.fr https://github.com/datagouv/udata-front/blob/9addd38e9523ff2e4898525a2787938c743f59f7/udata_front/theme/gouvfr/datagouv-components/src/components/DatasetCard/DatasetCard.vue#L3. Le badge actuel me parait un peu à l'étroit à sa place actuelle.
J'ai dû forcer l'icône du badge car il allait la chercher ici ../../icons/system/fr--warning-fill.svg et ne la trouvait pas.
Voir la solution data.gouv.fr plus haut pour un contournement (badge DSFR natif). Sinon il faut a minima ouvrir une issue sur vue-dsfr, voire faire une PR si possible.
J'ai également supprimé la prop max-height du Multiselect car ça causait un overflow lors de la sélection:
Est-ce que ça règle ecolabdata/ecospheres#233 ?
J'aime bien l'idée d'avertir l'utilisateur à la sélection, mais d'accord avec @abulte qu'un warning n'est pas le plus approprié. Peut-être le transformer en "info" ? +1 aussi sur l'emplacement du badge comme "Archivé" si possible. Pour ce qui est du texte, "Déjà sélectionné dans ce bouquet" serait effectivement plus explicite si tu arrives à le caser. Si on a vraiment la place, on pourrait même envisager d'indiquer sous quel(s) libellé(s) le dataset est déjà présent, pour que l'utilisateur puisse retrouver la/les autres occurrences plus facilement. Autre possibilité moins ergonomique : conserver "Déjà sélectionné" et ajouter un hover/infobulle avec une explication ? Si on a pas de bonne solution rapide, on peut aussi se contenter du minimum dans cette PR (fix ecolabdata/ecospheres#206) et revisiter le sujet après avoir calé les nouvelles orientations design avec Thomas. |
Effectivement, un badge info est plus adapté qu'un warning.
Ça me semble important d'avoir cette info pour éviter les erreurs de saisie. Pour le wording j'ai comme alternative: "Déjà présent" ou "Déjà utilisé" qui sont plus courts. À voir s'ils sont assez compréhensibles aussi. Il y a la possibilité de faire une ellipse automatique sur les petits écrans:
J'ai l'impression oui, j'ai testé firefox/chromium sur différentes tailles d'écrans ça a l'air ok. |
Top ! Tu peux l'ajouter en "Fix #..." dans la description du ticket ✌️
"sélectionné" ou "utilisé" (actifs) me semblent plus clair que "présent" (passif).
C'est un peu moche mais ça a le mérite d'être plus clair. On peut partir là dessus en attendant une review plus génerale du design. Ping @thomas-duport pour avis ? |
Le mieux serait de corriger 294 d'abord puis rebaser cette PR sur le fix pour ne pas avoir de pb d'overlap. Ca te semble jouable ? |
Oui carrément je me mets dessus ! |
Prevents it from overlaping on small screens
Still available to screen readers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deux choses :
- j'ai une erreur 400 via l'API quand j'essaie d'ajouter un jeu de données en double, je pense que c'est parce que les identifiants de jeux de données sont doublonnées dans la liste
Topic.datasets
, ce que data.gouv.fr ne supporte pas - il faudrait traiter le cas de l'ajout via la modale "Ajouter à un bouquet" depuis une page jeu de données, aujourd'hui je ne peux pas ajouter un jeu de données en double dans un bouquet par ce biais
Effectivement, l'ajout en base ne fonctionne pas… En revanche, en ajoutant le même DS depuis la page dudit DS, on peut l'ajouter plusieurs fois. Exemple ici avec le DS-2 et 2-bis.
J'ai ajouté la possibilité de sélectionner un bouquet qui utilise déjà le DS. J'ai rajouté le badge d'information en dessous une fois la sélection faite. |
pour préciser : il faut donc dédoublonner la liste avant de l'envoyer à data.gouv.fr, a priori ici
|
Ça fonctionne ! Ajout depuis la vue bouquet ou depuis la modal du dataset. Le bouquet est bien enregistré et conserve les datasets utilisés plusieurs fois. À priori pas d'erreur côté API 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En effet, ça marche !
Il faudrait juste changer le texte ici
:text="`Choisissez parmi les ${topicsName}s dont vous êtes l'auteur. Si un ${topicsName} apparait désactivé, c'est que le jeu de données y est déjà associé.`" |
Si un ${topicsName} apparait désactivé, c'est que le jeu de données y est déjà associé.
.
* Allow adding the same dataset multiple times to a bouquet. Add a warning on selection. Remove max-height on Multiselect. FIXME Forced badge icon from node_modules * Move keyboard instructions down Prevents it from overlaping on small screens * visually hide keyboard instruction Still available to screen readers * adjust styles of badge * Move badge intro card component * Add same DS to bouquet from DS detail * Deduplicate datasets before updating bouquet * cleanup import and text
max-height
duMultiselect
car ça causait un overflow lors de la sélection:../../icons/system/fr--warning-fill.svg
et ne la trouvait pas.Fix ecolabdata/ecospheres#206
fix ecolabdata/ecospheres#233